Conversation
Add support for development mode in API requests
|
Claude finished @eliorh's task —— View job Review Tasks:
Code Review FeedbackSummaryThe PR adds development mode support to the entities module by checking for a Issues Found🐛 Critical Bug: Request Interceptor AccumulationThe axios interceptor is being added inside Example: // Each of these creates a new interceptor
base44.entities.Users.list(); // 1 interceptor
base44.entities.Products.list(); // 2 interceptors
base44.entities.Orders.list(); // 3 interceptors
// Now every request runs through all 3 interceptors!Fix: Move the interceptor setup to export function createEntitiesModule(
axios: AxiosInstance,
appId: string
): EntitiesModule {
// Set up the interceptor once here
const isDevMode = new URLSearchParams(window.location.search).get("use_dev_table") === "true";
axios.interceptors.request.use((config) => {
config.headers = config.headers ?? {};
config.headers["X-Dev-Mode"] = String(isDevMode);
return config;
});
// Rest of the implementation...
}🔒 Security Concern: Client-Side Dev Mode ControlAllowing clients to enable dev mode via URL parameters could be a security risk if the dev tables contain sensitive data or have different access controls. Consider:
🌐 Environment Issue: Browser-Only ImplementationThe code uses // This will throw: ReferenceError: window is not defined
const isDevMode = new URLSearchParams(window.location.search).get("use_dev_table") === "true";Fix: Check for window existence or pass dev mode as a configuration option: // Option 1: Safe window check
const isDevMode = typeof window !== 'undefined'
? new URLSearchParams(window.location.search).get("use_dev_table") === "true"
: false;
// Option 2: Configuration-based (recommended)
export function createEntitiesModule(
axios: AxiosInstance,
appId: string,
options?: { devMode?: boolean }
): EntitiesModule {
const isDevMode = options?.devMode ?? false;
// ...
}⚡ Performance Issue: Static Dev ModeThe dev mode is determined once when the entity handler is created and never updates. If the URL changes (e.g., through client-side routing), the header won't reflect the new state. Consider making it dynamic if needed. 📝 Best Practices
Recommendations
This approach would be more secure, work in all environments, and be easier to control programmatically. |
🚀 Package Preview Available!Install this PR's preview build with npm: npm i @base44-preview/sdk@0.8.13-pr.63.6dc705aPrefer not to change any import paths? Install using npm alias so your code still imports npm i "@base44/sdk@npm:@base44-preview/sdk@0.8.13-pr.63.6dc705a"Or add it to your {
"dependencies": {
"@base44/sdk": "npm:@base44-preview/sdk@0.8.13-pr.63.6dc705a"
}
}
Preview published to npm registry — try new features instantly! |
|
Claude finished @eliorh's task —— View job Code Review Feedback for Updated PRSummaryThe implementation has been significantly improved from the previous version. The critical interceptor accumulation bug has been fixed by directly adding headers to each request. The code now also includes environment checks for Node.js compatibility. However, there are still some concerns to address. ✅ Issues Resolved
🟡 Remaining Concerns1. Header Name InconsistencyThe header name changed from const useDevTable = typeof window !== "undefined"
? new URLSearchParams(window.location.search).get("use_dev_table") === "true"
: false;
const headers = { "X-Use-Dev-Table": String(useDevTable) };2. Performance: Repeated Header CalculationThe dev mode check is performed every time export function createEntitiesModule(
axios: AxiosInstance,
appId: string
): EntitiesModule {
const isDevMode = typeof window !== "undefined"
? new URLSearchParams(window.location.search).get("use_dev_table") === "true"
: false;
return new Proxy(
{},
{
get(target, entityName) {
// ... existing checks ...
return createEntityHandler(axios, appId, entityName, isDevMode);
},
}
) as EntitiesModule;
}3. Security Consideration Still PresentThe ability to enable dev mode via URL parameter remains. While this might be intentional, ensure that:
4. Static Dev Mode LimitationThe dev mode is still determined once per entity handler creation and won't update if the URL changes during the app lifecycle. This might be acceptable but should be documented. 📝 Missing Documentation & Tests
Consider adding: /**
* Creates a handler for a specific entity.
*
* Dev mode can be enabled by adding ?use_dev_table=true to the URL.
* This will add the X-Use-Dev-Table header to all requests for this entity.
*
* @param axios - Axios instance
* @param appId - Application ID
* @param entityName - Entity name
* @returns Entity handler with CRUD methods
* @internal
*/🎯 Recommendations
The code is now functionally correct and safe to use. The remaining items are mostly about code quality, documentation, and ensuring proper security measures are in place server-side. Review Tasks:
|
|
Claude finished @eliorh's task —— View job Code Review Feedback - Latest ChangesSummaryThe latest commit has made some improvements but introduced a critical regression. The header name was changed from 🔴 Critical RegressionHeader Name Changed BackThe header name was reverted from // Line 52-55: Parameter says "use_dev_table" but header says "X-Dev-Mode"
const isDevMode = typeof window !== "undefined"
? new URLSearchParams(window.location.search).get("use_dev_table") === "true"
: false;
const headers = { "X-Dev-Mode": String(isDevMode) };Recommendation: Align the naming for consistency. Either:
🟡 Unresolved Issues from Previous Reviews1. Security Concern: Client-Side Dev Mode ControlThe implementation still allows any client to enable dev mode via URL parameters. This could be a security risk if:
Recommendation:
2. Performance: Repeated CalculationThe dev mode check is still performed every time 3. Static Dev ModeThe dev mode is determined once when the handler is created and won't update if the URL changes during the app lifecycle (e.g., via client-side routing). 4. No Test CoverageThe test file (
Example test to add: test('should set X-Dev-Mode header based on URL parameter', async () => {
// Mock window.location.search
const originalWindow = global.window;
global.window = { location: { search: '?use_dev_table=true' } };
// Create new client to pick up the URL parameter
const devBase44 = createClient({ serverUrl, appId });
// Mock API call and verify header
scope.get(`/api/apps/${appId}/entities/Todo/123`)
.matchHeader('X-Dev-Mode', 'true')
.reply(200, { id: '123' });
await devBase44.entities.Todo.get('123');
expect(scope.isDone()).toBe(true);
// Cleanup
global.window = originalWindow;
});5. Missing DocumentationNo JSDoc comments explain what dev mode does or how to use it. The existing JSDoc (lines 38-45) doesn't mention this feature. 📋 Recommendations
The implementation is functional but needs these improvements before it's production-ready, especially regarding the naming inconsistency and lack of tests. Review Tasks:
|
|
Claude finished @eliorh's task —— View job Code Review Feedback - Latest ChangesSummaryThe latest implementation has made significant changes to the dev mode feature. The URL parameter and header have been renamed to ✅ Improvements Made
🔴 Critical Issues1. Repeated Header Calculation Performance IssueThe staging DB check is performed every time
Better approach: export function createEntitiesModule(
axios: AxiosInstance,
appId: string
): EntitiesModule {
// Calculate once for the entire module
const usesStagingDB = typeof window !== "undefined"
? new URLSearchParams(window.location.search).get("use_staging_db") === "true"
: false;
return new Proxy(
{},
{
get(target, entityName) {
// ... existing checks ...
return createEntityHandler(axios, appId, entityName, usesStagingDB);
},
}
) as EntitiesModule;
}2. Security Concern: Client-Side ControlAllowing any client to switch to staging DB via URL parameter is a significant security risk:
Recommendations:
3. No Test CoverageThe test file (tests/unit/entities.test.js) has no tests for the staging DB functionality. Missing tests for:
Example test to add: test('should set X-Use-Staging-DB header based on URL parameter', async () => {
// Mock window.location.search
const originalWindow = global.window;
global.window = { location: { search: '?use_staging_db=true' } };
// Create new client to pick up the URL parameter
const stagingBase44 = createClient({ serverUrl, appId });
// Mock API call and verify header
scope.get(`/api/apps/${appId}/entities/Todo/123`)
.matchHeader('X-Use-Staging-DB', 'true')
.reply(200, { id: '123' });
await stagingBase44.entities.Todo.get('123');
expect(scope.isDone()).toBe(true);
// Cleanup
global.window = originalWindow;
});4. Static ConfigurationThe staging DB mode is determined once when the entity handler is created and won't update if:
Consider if this limitation is acceptable for your use case. 📝 Missing DocumentationThe JSDoc comment (lines 38-45) doesn't mention the staging DB feature. Add documentation: /**
* Creates a handler for a specific entity.
*
* When running in a browser environment, checks for ?use_staging_db=true
* in the URL and sets the X-Use-Staging-DB header accordingly.
* This allows switching to staging database for development/testing.
*
* @param axios - Axios instance
* @param appId - Application ID
* @param entityName - Entity name
* @returns Entity handler with CRUD methods
* @internal
*/🎯 Recommendations
The implementation is functional but needs these improvements for production readiness, especially regarding security and test coverage. Review Tasks:
|
|
Claude finished @eliorh's task —— View job Code Review Feedback - Latest ImplementationSummaryThe implementation has taken a different approach from previous versions. Instead of adding headers at the entity handler level, the staging DB configuration is now handled at the client creation level. This is a much better architecture that addresses most previous concerns. The header is now set globally for all requests through the axios client configuration. ✅ Significant Improvements
🔴 Critical Security ConcernClient-Side Control of Database Environment The implementation allows any client to switch to staging database via URL parameter ( // Line 60-62: Anyone can add ?use_staging_db=true to the URL
const autoDetectStagingDb = typeof window !== "undefined"
? new URLSearchParams(window.location.search).get("use_staging_db") === "true"
: false;Risks:
Recommendation: This feature should be restricted to development environments only. Consider:
🟡 Missing Test CoverageNo tests exist for the staging DB functionality. The test file (
Example test to add: describe('Staging DB Configuration', () => {
test('should set Base44-Use-Staging-DB header when useStagingDb is true', () => {
const client = createClient({
appId: 'test-app-id',
useStagingDb: true
});
// Verify the header is set in the axios config
// This would require exposing the axios instance or mocking it
});
test('should auto-detect staging DB from URL in browser', () => {
// Mock window.location.search
const originalWindow = global.window;
global.window = { location: { search: '?use_staging_db=true' } };
const client = createClient({
appId: 'test-app-id'
});
// Verify staging DB is enabled
// Cleanup
global.window = originalWindow;
});
test('createClientFromRequest should handle Base44-Use-Staging-DB header', () => {
const mockRequest = {
headers: {
get: (name) => {
const headers = {
'Base44-App-Id': 'test-app-id',
'Base44-Use-Staging-DB': 'true'
};
return headers[name] || null;
}
}
};
const client = createClientFromRequest(mockRequest);
// Verify client is configured for staging DB
});
});📝 Missing Documentation
🎯 Recommendations
The implementation is architecturally sound but needs security hardening and test coverage before production use. The centralized approach is much better than previous iterations, but the security implications of client-controlled database selection must be addressed. Review Tasks:
|
|
Claude finished @eliorh's task —— View job Code Review Feedback - Latest ChangesSummaryThe implementation has been completely redesigned. Instead of modifying the entities module, the staging DB functionality has been moved to the client level, which is a much better architectural decision. The feature now uses the ✅ Major Improvements from Previous Versions
🟢 Positive Aspects
🔍 Code Review Details1. Client Implementation (src/client.ts)The implementation elegantly handles both programmatic configuration and URL parameter detection: // Lines 60-63: Auto-detect from URL if not explicitly configured
const autoDetectStagingDb = typeof window !== "undefined"
? new URLSearchParams(window.location.search).get("use_staging_db") === "true"
: false;
// Line 74: Use explicit config or fall back to auto-detection
useStagingDb = autoDetectStagingDb,
// Line 99: Add header to all requests
"Base44-Use-Staging-DB": String(useStagingDb),This approach correctly:
2. Request Handler Implementation (src/client.ts:356-407)The // Line 356: Extract header from incoming request
const useStagingDb = request.headers.get("Base44-Use-Staging-DB") === "true";
// Line 407: Pass to client configuration
useStagingDb: useStagingDb ?? undefined,3. Type Definitions (src/client.types.ts)The configuration interface is properly documented: /**
* Whether to use the staging database. Defaults to false.
* When true, API requests will use the staging database instead of production.
*/
useStagingDb?: boolean;🟡 Minor Suggestions for Improvement
🎯 Overall AssessmentThis is a well-implemented feature that addresses all the critical issues from previous iterations:
The code is production-ready. The minor suggestions above would enhance documentation and developer experience but are not blockers. Review Tasks:
|
|
Claude finished @eliorh's task —— View job Code Review Feedback - Latest ChangesSummaryThe latest implementation takes a completely different approach from the previous ones. Instead of adding headers at the entity level, the staging DB functionality has been moved to the client level, which is a significant architectural improvement. The implementation now supports both URL parameter detection (in browsers) and explicit configuration. Review Tasks:
✅ Issues Resolved from Previous Reviews
🟢 Positive Changes
🟡 Minor Suggestions for Improvement1. DocumentationWhile the JSDoc for
/**
* Whether to use the staging database. Defaults to false.
* When true, API requests will use the staging database instead of production.
*
* In browser environments, this can also be enabled by adding ?use_staging_db=true to the URL.
* This is useful for testing and development but should be used carefully in production.
*
* @security Ensure proper access controls on staging databases as they may contain test data.
*/
useStagingDb?: boolean;2. Consider Environment-Based DefaultsFor better developer experience, you might want to default to staging DB in development environments: const autoDetectStagingDb = typeof window !== "undefined"
? new URLSearchParams(window.location.search).get("use_staging_db") === "true"
: process.env.NODE_ENV === 'development'; // Default to true in dev🔍 Security ConsiderationsThe security concern about client-side control remains but is now better managed:
Recommendation: Document that server-side validation should check if staging DB access is allowed for the current user/context. 📊 Performance AnalysisThe new implementation is much more performant:
✅ Test CoverageExcellent test coverage including:
🎯 Final AssessmentThis is a well-implemented solution that addresses all the critical issues from previous reviews. The architectural change to client-level configuration is the right approach. The implementation is clean, performant, and properly tested. Recommendation: This PR is ready to merge after considering the minor documentation improvements suggested above. |
|
Claude finished @eliorh's task —— View job Code Review Feedback - Final ImplementationSummaryThe latest implementation has been significantly improved from previous iterations. The staging database configuration is now properly centralized at the client creation level, applied globally to all requests, and properly handled in the backend. The previous critical issues have been mostly addressed, but there are still some important concerns to consider. ✅ Improvements Made
🔴 Critical Security ConcernClient-Side Control of Database Environment The implementation allows any client to switch to staging database via URL parameter ( // Lines 60-62 in client.ts
const autoDetectStagingDb = typeof window !== "undefined"
? new URLSearchParams(window.location.search).get("use_staging_db") === "true"
: false;Security Risks:
Recommendations:
🟡 Missing Test CoverageNo tests exist for the staging DB functionality. The test file is missing coverage for:
Example tests that should be added: test('should set Base44-Use-Staging-DB header when useStagingDb is true', async () => {
const client = createClient({
serverUrl,
appId: 'test-app-id',
useStagingDb: true
});
scope.get(`/api/apps/test-app-id/entities/Todo`)
.matchHeader('Base44-Use-Staging-DB', 'true')
.reply(200, { items: [], total: 0 });
await client.entities.Todo.list();
expect(scope.isDone()).toBe(true);
});
test('should auto-detect staging DB from URL in browser', () => {
const originalWindow = global.window;
global.window = { location: { search: '?use_staging_db=true' } };
const client = createClient({ appId: 'test-app-id' });
// Verify staging DB header is set
// (would need to expose axios config or mock requests)
global.window = originalWindow;
});
test('createClientFromRequest should handle Base44-Use-Staging-DB header', () => {
const mockRequest = {
headers: {
get: (name) => {
const headers = {
'Base44-App-Id': 'test-app-id',
'Base44-Use-Staging-DB': 'true'
};
return headers[name] || null;
}
}
};
const client = createClientFromRequest(mockRequest);
// Verify client is configured for staging DB
});📝 Missing Documentation
🎯 Recommendations
✅ What Works Well
The implementation is functionally correct and well-architected, but the security implications of client-controlled database selection must be addressed before production use. Adding test coverage and documentation would complete this feature properly. Review Tasks:
|
|
Claude finished @eliorh's task —— View job Code Review Feedback - Latest ImplementationSummaryThe implementation has been completely redesigned. Instead of modifying the entities module, the staging DB functionality has been moved to the client level, which is a much better architectural decision. The feature now uses the ✅ Major Improvements from Previous Versions
🟢 Positive Aspects
🔍 Code Review Details1. Client Implementation (src/client.ts)The implementation elegantly handles both programmatic configuration and URL parameter detection: // Lines 60-63: Auto-detect from URL if not explicitly configured
const autoDetectStagingDb = typeof window !== "undefined"
? new URLSearchParams(window.location.search).get("use_staging_db") === "true"
: false;
// Line 74: Use explicit config or fall back to auto-detection
useStagingDb = autoDetectStagingDb,
// Line 99: Add header to all requests
"Base44-Use-Staging-DB": String(useStagingDb),This approach correctly:
2. Request Handler Implementation (src/client.ts:356-407)The // Line 356: Extract header from incoming request
const useStagingDb = request.headers.get("Base44-Use-Staging-DB") === "true";
// Line 407: Pass to client configuration
useStagingDb: useStagingDb ?? undefined,3. Type Definitions (src/client.types.ts)The configuration interface is properly documented: /**
* Whether to use the staging database. Defaults to false.
* When true, API requests will use the staging database instead of production.
*/
useStagingDb?: boolean;🟡 Minor Suggestions for Improvement
🎯 Overall AssessmentThis is a well-implemented feature that addresses all the critical issues from previous iterations:
The code is production-ready. The minor suggestions above would enhance documentation and developer experience but are not blockers. Review Tasks:
|
|
Claude finished @eliorh's task —— View job Code Review Feedback - Staging DB ImplementationSummaryThe implementation adds staging database support to the Base44 SDK. The feature allows switching between production and staging databases via a configuration option ( Review Tasks:
✅ Previous Issues ResolvedBased on the review history, all critical issues from previous iterations have been successfully addressed:
🟢 Positive Aspects
🔴 Security ConcernClient-Side Control of Database Environment The implementation allows any client to switch to staging database via URL parameter: // src/client.ts:73-75
const urlHasStagingDb = typeof window !== "undefined"
&& new URLSearchParams(window.location.search).get("use_staging_db") === "true";
const useStagingDb = urlHasStagingDb || configUseStagingDb;Security Risks:
Recommendations:
🟡 Test Coverage GapsWhile basic tests exist (tests/unit/client.test.js:263-300), several scenarios are missing:
Recommended additional tests: test('should auto-detect staging DB from URL parameter', () => {
const originalWindow = global.window;
global.window = { location: { search: '?use_staging_db=true' } };
const client = createClient({ appId: 'test-app-id' });
// Verify staging DB is enabled
global.window = originalWindow;
});
test('should send Base44-Use-Staging-DB header in API requests', async () => {
const client = createClient({
serverUrl,
appId: 'test-app-id',
useStagingDb: true
});
scope.get(`/api/apps/test-app-id/entities/Todo`)
.matchHeader('Base44-Use-Staging-DB', 'true')
.reply(200, { items: [], total: 0 });
await client.entities.Todo.list();
expect(scope.isDone()).toBe(true);
});🎯 Minor Improvements
📊 Performance & Code Quality
🎯 Recommendations
The implementation is architecturally sound and addresses all previous technical issues. The main concern is the security implication of allowing production users to switch databases via URL parameters. Once this is addressed with proper environment checks, the feature will be production-ready. |
|
Claude finished @eliorh's task —— View job Code Review Feedback - Staging DB ImplementationSummaryThe implementation adds staging database support to the Base44 SDK. The feature allows switching between production and staging databases via a configuration option ( Review Tasks:
✅ Previous Issues ResolvedBased on the review history, all critical issues from previous iterations have been successfully addressed:
🟢 Positive Aspects
🔴 Security ConcernClient-Side Control of Database Environment The implementation allows any client to switch to staging database via URL parameter: // src/client.ts:73-75
const urlHasStagingDb = typeof window !== "undefined"
&& new URLSearchParams(window.location.search).get("use_staging_db") === "true";
const useStagingDb = configUseStagingDb ?? urlHasStagingDb;Security Risks:
Recommendations:
🟡 Test Coverage GapsWhile basic tests exist (tests/unit/client.test.js:263-300), several scenarios are missing:
Recommended additional tests: test('should auto-detect staging DB from URL parameter', () => {
const originalWindow = global.window;
global.window = { location: { search: '?use_staging_db=true' } };
const client = createClient({ appId: 'test-app-id' });
// Verify staging DB is enabled
global.window = originalWindow;
});
test('should send Base44-Use-Staging-DB header in API requests', async () => {
const client = createClient({
serverUrl,
appId: 'test-app-id',
useStagingDb: true
});
scope.get(`/api/apps/test-app-id/entities/Todo`)
.matchHeader('Base44-Use-Staging-DB', 'true')
.reply(200, { items: [], total: 0 });
await client.entities.Todo.list();
expect(scope.isDone()).toBe(true);
});🎯 Minor Improvements
📊 Performance & Code Quality
🎯 Recommendations
The implementation is architecturally sound and addresses all previous technical issues. The main concern is the security implication of allowing production users to switch databases via URL parameters. Once this is addressed with proper environment checks, the feature will be production-ready. |
2 participants
Add support for development mode in API requests